Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security group retries #9812

Merged
merged 4 commits into from
Aug 21, 2019
Merged

Security group retries #9812

merged 4 commits into from
Aug 21, 2019

Conversation

ryndaniels
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates #7873

Release note for CHANGELOG:

BUG FIXES:
* resource/aws_security_group: Final retry after timeout deleting security group
* resource/aws_security_group_rule: Final retry after timeout creating security group rule

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSecurityGroup'

--- PASS: TestAccAWSSecurityGroup_namePrefix (18.49s)
--- PASS: TestAccAWSSecurityGroup_basic (24.95s)
--- PASS: TestAccAWSSecurityGroup_vpc (28.56s)
--- PASS: TestAccAWSSecurityGroup_self (31.39s)
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (33.24s)
--- PASS: TestAccAWSSecurityGroup_importIpv6 (33.53s)
--- PASS: TestAccAWSSecurityGroup_MultiIngress (34.07s)
--- SKIP: TestAccAWSSecurityGroup_DefaultEgress_Classic (1.27s)
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (1.54s)
--- PASS: TestAccAWSSecurityGroup_ipv6 (41.63s)
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (43.93s)
--- PASS: TestAccAWSSecurityGroup_importSelf (49.10s)
--- SKIP: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (1.08s)
--- PASS: TestAccAWSSecurityGroup_generatedName (21.67s)
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_VPC (18.96s)
--- PASS: TestAccAWSSecurityGroup_importIPRangesWithSameRules (50.93s)
--- PASS: TestAccAWSSecurityGroup_importSourceSecurityGroup (54.09s)
--- PASS: TestAccAWSSecurityGroup_ruleGathering (56.87s)
--- PASS: TestAccAWSSecurityGroup_importBasic (57.19s)
--- PASS: TestAccAWSSecurityGroup_drift (26.57s)
--- PASS: TestAccAWSSecurityGroup_importPrefixList (71.47s)
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (26.60s)
--- PASS: TestAccAWSSecurityGroup_drift_complex (48.79s)
--- PASS: TestAccAWSSecurityGroup_ingressWithPrefixList (33.52s)
--- PASS: TestAccAWSSecurityGroup_Change (68.55s)
--- PASS: TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules (87.63s)
--- PASS: TestAccAWSSecurityGroup_Egress_ConfigMode (89.60s)
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (42.79s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend (39.49s)
--- PASS: TestAccAWSSecurityGroup_Ingress_ConfigMode (97.10s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAppend (50.16s)
--- PASS: TestAccAWSSecurityGroup_rulesDropOnError (32.88s)
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (55.87s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (64.23s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededPrepend (52.41s)
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (68.15s)
--- PASS: TestAccAWSSecurityGroup_RuleDescription (85.12s)
--- PASS: TestAccAWSSecurityGroup_tags (74.72s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAllNew (61.32s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_false (1251.66s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_true (1310.60s)


make testacc TESTARGS="-run=TestAccAWSSecurityGroupRule"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSSecurityGroupRule -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSSecurityGroupRule_Ingress_VPC
=== PAUSE TestAccAWSSecurityGroupRule_Ingress_VPC
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Protocol
=== PAUSE TestAccAWSSecurityGroupRule_Ingress_Protocol
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Ipv6
=== PAUSE TestAccAWSSecurityGroupRule_Ingress_Ipv6
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Classic
=== PAUSE TestAccAWSSecurityGroupRule_Ingress_Classic
=== RUN   TestAccAWSSecurityGroupRule_MultiIngress
=== PAUSE TestAccAWSSecurityGroupRule_MultiIngress
=== RUN   TestAccAWSSecurityGroupRule_Egress
=== PAUSE TestAccAWSSecurityGroupRule_Egress
=== RUN   TestAccAWSSecurityGroupRule_SelfReference
=== PAUSE TestAccAWSSecurityGroupRule_SelfReference
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
=== PAUSE TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
=== PAUSE TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_basic
=== PAUSE TestAccAWSSecurityGroupRule_PartialMatching_basic
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_Source
=== PAUSE TestAccAWSSecurityGroupRule_PartialMatching_Source
=== RUN   TestAccAWSSecurityGroupRule_Issue5310
=== PAUSE TestAccAWSSecurityGroupRule_Issue5310
=== RUN   TestAccAWSSecurityGroupRule_Race
=== PAUSE TestAccAWSSecurityGroupRule_Race
=== RUN   TestAccAWSSecurityGroupRule_SelfSource
=== PAUSE TestAccAWSSecurityGroupRule_SelfSource
=== RUN   TestAccAWSSecurityGroupRule_PrefixListEgress
=== PAUSE TestAccAWSSecurityGroupRule_PrefixListEgress
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription
=== PAUSE TestAccAWSSecurityGroupRule_IngressDescription
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription
=== PAUSE TestAccAWSSecurityGroupRule_EgressDescription
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription_updates
=== PAUSE TestAccAWSSecurityGroupRule_IngressDescription_updates
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription_updates
=== PAUSE TestAccAWSSecurityGroupRule_EgressDescription_updates
=== RUN   TestAccAWSSecurityGroupRule_Description_AllPorts
=== PAUSE TestAccAWSSecurityGroupRule_Description_AllPorts
=== RUN   TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts
=== PAUSE TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts
=== RUN   TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash
=== PAUSE TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash
=== RUN   TestAccAWSSecurityGroupRule_MultiDescription
=== PAUSE TestAccAWSSecurityGroupRule_MultiDescription
=== CONT  TestAccAWSSecurityGroupRule_Ingress_VPC
=== CONT  TestAccAWSSecurityGroupRule_MultiDescription
=== CONT  TestAccAWSSecurityGroupRule_PartialMatching_Source
=== CONT  TestAccAWSSecurityGroupRule_Issue5310
=== CONT  TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash
=== CONT  TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts
=== CONT  TestAccAWSSecurityGroupRule_Description_AllPorts
=== CONT  TestAccAWSSecurityGroupRule_EgressDescription_updates
=== CONT  TestAccAWSSecurityGroupRule_IngressDescription_updates
=== CONT  TestAccAWSSecurityGroupRule_EgressDescription
=== CONT  TestAccAWSSecurityGroupRule_IngressDescription
=== CONT  TestAccAWSSecurityGroupRule_PrefixListEgress
=== CONT  TestAccAWSSecurityGroupRule_Race
=== CONT  TestAccAWSSecurityGroupRule_Ingress_Classic
=== CONT  TestAccAWSSecurityGroupRule_Egress
=== CONT  TestAccAWSSecurityGroupRule_SelfReference
=== CONT  TestAccAWSSecurityGroupRule_SelfSource
=== CONT  TestAccAWSSecurityGroupRule_Ingress_Ipv6
=== CONT  TestAccAWSSecurityGroupRule_MultiIngress
=== CONT  TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (3.70s)
=== CONT  TestAccAWSSecurityGroupRule_Ingress_Protocol
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (37.19s)
=== CONT  TestAccAWSSecurityGroupRule_PartialMatching_basic
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (38.19s)
=== CONT  TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
--- PASS: TestAccAWSSecurityGroupRule_Egress (38.24s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (38.25s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (38.31s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (38.31s)
--- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (39.20s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (2.28s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (43.16s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (57.82s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (58.07s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (58.70s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts (60.09s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (63.41s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (63.56s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (65.61s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (63.94s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (70.40s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (80.94s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (73.33s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (147.77s)
--- PASS: TestAccAWSSecurityGroupRule_Race (476.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       477.371s

@ryndaniels ryndaniels requested a review from a team August 19, 2019 11:50
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 19, 2019
@ryndaniels ryndaniels added the bug Addresses a defect in current functionality. label Aug 19, 2019
@ryndaniels ryndaniels added this to the v2.25.0 milestone Aug 19, 2019
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two style changes, otherwise LGTM 🚀

--- PASS: TestIpPermissionIDHash (0.00s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (4.50s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (6.55s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (19.54s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.10s)
--- PASS: TestProtocolStateFunc (0.00s)
--- PASS: TestProtocolForValue (0.00s)
--- PASS: TestResourceAwsSecurityGroupExpandCollapseRules (0.00s)
--- PASS: TestResourceAwsSecurityGroupIPPermGather (0.00s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.45s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (27.54s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (28.16s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (28.03s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (28.76s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (30.07s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (30.33s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (35.06s)
--- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (30.01s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (46.79s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts (44.22s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (54.82s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (54.30s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (55.38s)
--- PASS: TestAccAWSSecurityGroup_importIpv6 (36.24s)
--- PASS: TestAccAWSSecurityGroup_importBasic (39.60s)
--- PASS: TestAccAWSSecurityGroup_basic (32.27s)
--- PASS: TestAccAWSSecurityGroup_namePrefix (13.63s)
--- PASS: TestAccAWSSecurityGroup_importSourceSecurityGroup (42.30s)
--- PASS: TestAccAWSSecurityGroup_importIPRangesWithSameRules (44.30s)
--- PASS: TestAccAWSSecurityGroup_ipv6 (19.19s)
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (22.70s)
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (29.29s)
--- PASS: TestAccAWSSecurityGroup_self (42.91s)
--- PASS: TestAccAWSSecurityGroup_Egress_ConfigMode (65.41s)
--- SKIP: TestAccAWSSecurityGroup_DefaultEgress_Classic (1.12s)
--- PASS: TestAccAWSSecurityGroup_importPrefixList (73.11s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (103.52s)
--- PASS: TestAccAWSSecurityGroup_MultiIngress (34.03s)
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (1.98s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (90.65s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (110.34s)
--- PASS: TestAccAWSSecurityGroup_ruleGathering (63.73s)
--- SKIP: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (1.07s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (112.66s)
--- PASS: TestAccAWSSecurityGroup_generatedName (25.60s)
--- PASS: TestAccAWSSecurityGroup_drift (24.95s)
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_VPC (33.87s)
--- PASS: TestAccAWSSecurityGroup_drift_complex (30.01s)
--- PASS: TestAccAWSSecurityGroup_Change (64.62s)
--- PASS: TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules (109.72s)
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (37.33s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (38.63s)
--- PASS: TestAccAWSSecurityGroup_importSelf (119.81s)
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (39.14s)
--- PASS: TestAccAWSSecurityGroup_Ingress_ConfigMode (123.24s)
--- PASS: TestAccAWSSecurityGroup_ingressWithPrefixList (52.98s)
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (52.89s)
--- PASS: TestAccAWSSecurityGroup_tags (63.39s)
--- PASS: TestAccAWSSecurityGroup_RuleDescription (93.59s)
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (65.30s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAppend (60.20s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend (54.50s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededPrepend (57.71s)
--- PASS: TestAccAWSSecurityGroup_rulesDropOnError (48.32s)
--- PASS: TestAccAWSSecurityGroup_vpc (128.12s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAllNew (57.76s)
--- PASS: TestAccAWSSecurityGroupRule_Race (200.76s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_false (1252.95s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_true (1293.55s)

aws/resource_aws_security_group.go Outdated Show resolved Hide resolved
aws/resource_aws_security_group.go Outdated Show resolved Hide resolved
@ryndaniels ryndaniels merged commit 1f4d45a into master Aug 21, 2019
@ryndaniels ryndaniels deleted the rfd-retry-security branch August 21, 2019 13:42
@ghost
Copy link

ghost commented Aug 23, 2019

This has been released in version 2.25.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

bflad added a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Oct 8, 2019
…yableError

Reference: hashicorp/terraform#17220
Reference: hashicorp/terraform-provider-aws#9779 (comment)
Reference: hashicorp/terraform-provider-aws#9812 (comment)

It is currently possible to introduce subtle bugs or crashes when using `resource.RetryableError` and `resource.NonRetryableError` and allowing a `nil` error input. This PR proposes behavior that requires providers to be explicit with their usage of these errors, otherwise returns a bug reporting message to the operator.

For example (https://github.com/terraform-providers/terraform-provider-aws/blob/4c0387645f982ddcd51b3ffe2cc8992c06fb9c2c/aws/resource_aws_elastic_beanstalk_application.go#L105):

```go
	var app *elasticbeanstalk.ApplicationDescription
	err := resource.Retry(30*time.Second, func() *resource.RetryError {
		var err error
		app, err = getBeanstalkApplication(d.Id(), conn)
		if err != nil {
			return resource.NonRetryableError(err)
		}

		if app == nil {
			if d.IsNewResource() {
				return resource.RetryableError(fmt.Errorf("Elastic Beanstalk Application %q not found", d.Id()))
			}
			// err is nil here
			return resource.NonRetryableError(err)
		}
		return nil
	})
	if err != nil {
		return err
	}
	// app can be nil here, so this can crash Terraform
	d.Set("name", app.ApplicationName)
```

Another example (https://github.com/terraform-providers/terraform-provider-alicloud/blob/927a8e82386e0b32718aa5eef254255fdc36b070/alicloud/resource_alicloud_disk_attachment.go#L112):

```go
	return resource.Retry(5*time.Minute, func() *resource.RetryError {
		err := conn.DetachDisk(instanceID, diskID)
		if err != nil {
			if IsExceptedError(err, DiskIncorrectStatus) || IsExceptedError(err, InstanceLockedForSecurity) ||
				IsExceptedError(err, DiskInvalidOperation) {
				return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err))
			}
		}

		disks, _, descErr := conn.DescribeDisks(&ecs.DescribeDisksArgs{
			RegionId: getRegion(d, meta),
			DiskIds:  []string{diskID},
		})

		if descErr != nil {
			log.Printf("[ERROR] Disk %s is not detached.", diskID)
			// err can be nil here
			return resource.NonRetryableError(err)
		}

		for _, disk := range disks {
			if disk.Status != ecs.DiskStatusAvailable {
				return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err))
			}
		}
		return nil
	})
```

Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/75c32b375140813b7994f8031e74b8588a08035a/aws/resource_aws_kms_alias.go#L176-L190):

```go
func retryFindKmsAliasByName(conn *kms.KMS, name string) (*kms.AliasListEntry, error) {
	var resp *kms.AliasListEntry
	err := resource.Retry(1*time.Minute, func() *resource.RetryError {
		var err error
		resp, err = findKmsAliasByName(conn, name, nil)
		if err != nil {
			return resource.NonRetryableError(err)
		}
		if resp == nil {
			// err is nil here, so returns nil
			return resource.RetryableError(err)
		}
		return nil
	})
	return resp, err
}
```

Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/00909998d919faf5494ab8f6b38241deb1957d99/aws/resource_aws_internet_gateway.go#L55-L65):

```go
	err = resource.Retry(5*time.Minute, func() *resource.RetryError {
		igRaw, _, err := IGStateRefreshFunc(conn, d.Id())()
		if igRaw != nil {
			return nil
		}
		if err == nil {
			// err is always nil here, so it will never retry
			return resource.RetryableError(err)
		} else {
			return resource.NonRetryableError(err)
		}
	})
```
@ghost
Copy link

ghost commented Nov 1, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants